Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

combine keep_attrs and combine_attrs in apply_ufunc #5041

Merged
merged 22 commits into from
May 13, 2021
Merged

combine keep_attrs and combine_attrs in apply_ufunc #5041

merged 22 commits into from
May 13, 2021

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Mar 16, 2021 •
edited
Loading

As discussed this extends keep_attrs in apply_ufunc to also accept merge strategy names and use merge_attrs (making it easier to add more merge strategies).

Handling attrs on variables / coords is still missing.

  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

@keewis
Copy link
Collaborator Author

keewis commented Mar 25, 2021

it seems apply_ufunc already applies keep_attrs/combine_attrs on the data variables, but not the coords. Not sure if we should try to combine attrs on coords, too (should be fixed by adding a keep_attrs kwarg to align / deep_align)

@keewis keewis mentioned this pull request Apr 3, 2021
Merged
3 tasks
max-sixty
max-sixty approved these changes Apr 18, 2021
View reviewed changes
Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong view on how we combine on the coords. Though is there any reason to wait on merging this? The tests are great!

xarray/tests/test_computation.py Show resolved Hide resolved
@keewis keewis mentioned this pull request Apr 19, 2021
Merged
4 tasks
@dcherian
Copy link
Contributor

if we should try to combine attrs on coord

👍 on this. can fix later though...

@keewis
Copy link
Collaborator Author

keewis commented Apr 19, 2021

apply_ufunc ultimately calls merge_collected to determine the output coords so we would need #4902 to also keep the attrs on coords.

@keewis
Copy link
Collaborator Author

keewis commented May 5, 2021

this is ready, too. Not sure if we should merge before the release, though.

@max-sixty
Copy link
Collaborator

this is ready, too. Not sure if we should merge before the release, though.

As good a time as any! But as ever your call on whether we can be sufficiently confident

keewis
keewis commented May 5, 2021
View reviewed changes
Copy link
Collaborator Author

@keewis keewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering that we originally planned to release tomorrow (?) I'd say let's wait and include this in the next release. I just went through the code and found a few issues which need more thought (I think?)

xarray/core/computation.py Outdated Show resolved Hide resolved
@keewis keewis mentioned this pull request May 8, 2021
Merged
1 task
@keewis
Copy link
Collaborator Author

keewis commented May 8, 2021

not sure if I got all of the issues I thought I noticed, but I think this should be ready

max-sixty
max-sixty approved these changes May 8, 2021
View reviewed changes
Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good!

(Whatsnew needs switching versions FYI)

@keewis
Copy link
Collaborator Author

keewis commented May 8, 2021

(Whatsnew needs switching versions FYI)

thanks, I completely missed that

@dcherian
Copy link
Contributor

Thanks @keewis. I found the compute_attrs thing a little complicated and hard to understand. But I think it's OK and am going to merge now. If you have a clever idea for something clearer, we can address that later.

Thanks!

@dcherian dcherian merged commit 751f76a into pydata:master May 13, 2021
dcherian added a commit to TomNicholas/xarray that referenced this pull request May 13, 2021
@dcherian
Merge remote-tracking branch 'upstream/master' into update_release_guide
a9cbf80 
* upstream/master:
  combine keep_attrs and combine_attrs in apply_ufunc (pydata#5041)
  Explained what a deprecation cycle is (pydata#5289)
  Code cleanup (pydata#5234)
  FacetGrid docstrings (pydata#5293)
  Add whats new for dataset interpolation with non-numerics (pydata#5297)
  Allow dataset interpolation with different datatypes (pydata#5008)
  Flexible indexes: add Index base class and xindexes properties (pydata#5102)
  pre-commit: autoupdate hook versions (pydata#5280)
  convert the examples for apply_ufunc to doctest (pydata#5279)
  fix the new whatsnew section
  Ensure `HighLevelGraph` layers are `Layer` instances (pydata#5271)
@keewis keewis deleted the keep_attrs branch May 13, 2021 17:33
dcherian added a commit to matzegoebel/xarray that referenced this pull request May 13, 2021
@dcherian
Merge remote-tracking branch 'upstream/master' into dev
74568b6 
* upstream/master: (23 commits)
  combine keep_attrs and combine_attrs in apply_ufunc (pydata#5041)
  Explained what a deprecation cycle is (pydata#5289)
  Code cleanup (pydata#5234)
  FacetGrid docstrings (pydata#5293)
  Add whats new for dataset interpolation with non-numerics (pydata#5297)
  Allow dataset interpolation with different datatypes (pydata#5008)
  Flexible indexes: add Index base class and xindexes properties (pydata#5102)
  pre-commit: autoupdate hook versions (pydata#5280)
  convert the examples for apply_ufunc to doctest (pydata#5279)
  fix the new whatsnew section
  Ensure `HighLevelGraph` layers are `Layer` instances (pydata#5271)
  New whatsnew section
  Release-workflow: Bug fix (pydata#5273)
  more maintenance on whats-new.rst (pydata#5272)
  v0.18.0 release highlights (pydata#5266)
  Fix exception when display_expand_data=False for file-backed array. (pydata#5235)
  Warn ignored keep attrs (pydata#5265)
  Disable workflows on forks (pydata#5267)
  fix the built wheel test (pydata#5270)
  pypi upload workflow maintenance (pydata#5269)
  ...
dcherian added a commit to gcaria/xarray that referenced this pull request May 13, 2021
@dcherian
Merge remote-tracking branch 'upstream/master' into error_message_tim…
c33fa82 
…e_units

* upstream/master:
  combine keep_attrs and combine_attrs in apply_ufunc (pydata#5041)
  Explained what a deprecation cycle is (pydata#5289)
  Code cleanup (pydata#5234)
  FacetGrid docstrings (pydata#5293)
  Add whats new for dataset interpolation with non-numerics (pydata#5297)
  Allow dataset interpolation with different datatypes (pydata#5008)
  Flexible indexes: add Index base class and xindexes properties (pydata#5102)
dcherian added a commit to ahuang11/xarray that referenced this pull request May 13, 2021
@dcherian
Merge remote-tracking branch 'upstream/master' into drop_duplicates_dim
2e8dbd3 
* upstream/master:
  Update release guide (pydata#5274)
  combine keep_attrs and combine_attrs in apply_ufunc (pydata#5041)
  Explained what a deprecation cycle is (pydata#5289)
  Code cleanup (pydata#5234)
  FacetGrid docstrings (pydata#5293)
  Add whats new for dataset interpolation with non-numerics (pydata#5297)
  Allow dataset interpolation with different datatypes (pydata#5008)
@arongergely arongergely mentioned this pull request Jan 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-sixty max-sixty max-sixty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@keewis @dcherian @max-sixty